feat: add test_helpers module (error_utils, test_utils) behind function_runner flag#2378
feat: add test_helpers module (error_utils, test_utils) behind function_runner flag#2378naor-starkware wants to merge 8 commits intonaor/refactor/rename-feature-flagfrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## naor/refactor/rename-feature-flag #2378 +/- ##
=====================================================================
- Coverage 96.07% 96.06% -0.01%
=====================================================================
Files 105 107 +2
Lines 37737 37968 +231
=====================================================================
+ Hits 36254 36475 +221
- Misses 1483 1493 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Benchmark Results for unmodified programs 🚀
|
6ec5328 to
ce3aeb1
Compare
OmriEshhar1
left a comment
There was a problem hiding this comment.
@OmriEshhar1 reviewed 7 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on naor-starkware).
…on_runner flag - Create vm/src/test_helpers/ with error_utils.rs and test_utils.rs - Move from cairo_test_suite/ (fix filename typo: utlis → utils) - Fix crate:: import paths (were cairo_vm:: when outside the crate) - Fix $crate in macro_export macro (clippy::crate_in_macro_def) - Simplify load_cairo_program! path using with_file_name() - Gate module behind function_runner feature in lib.rs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ram! and error_utils checkers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add AlwaysFailConversion helper + 2 tests for assert_mr_eq! unwrap_or_else panic branch (no-message and message variants) - Allow clippy::result_large_err on hint_err test helper Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… noise Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y function name error #[macro_export] macros containing closures (|x| ...) cause llvm-cov to emit a "function name is empty" error. Replaced unwrap_or_else(|e| panic!(...)) with match expressions to eliminate closures from macro expansions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Follow-up to dropping the function_runner feature flag. Gate test_helpers module and function_runner module under test_utils, and update the doc comment in function_runner.rs accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
e97f3a8 to
3e8d6ce
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
YairVaknin-starkware
left a comment
There was a problem hiding this comment.
@YairVaknin-starkware reviewed 8 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 5 unresolved discussions.
CHANGELOG.md line 17 at r2 (raw file):
* refactor: add `CairoFunctionRunner` type alias for `CairoRunner` under the `test_utils` feature flag [#2377](https://github.com/starkware-libs/cairo-vm/pull/2377) * refactor: add `function_runner` feature flag and `CairoFunctionRunner` type alias for `CairoRunner` [#2377](https://github.com/starkware-libs/cairo-vm/pull/2377) * feat: add `test_helpers` module (`error_utils`, `test_utils`) with `assert_mr_eq!`, `load_cairo_program!` macros and `expect_*` error checkers, behind `test_utils` feature flag [#2378](https://github.com/starkware-libs/cairo-vm/pull/2378)
fix
Code quote:
* refactor: add `function_runner` feature flag and `CairoFunctionRunner` type alias for `CairoRunner` [#2377](https://github.com/starkware-libs/cairo-vm/pull/2377)
* feat: add `test_helpers` module (`error_utils`, `test_utils`) with `assert_mr_eq!`, `load_cairo_program!` macros and `expect_*` error checkers, behind `test_utils` feature flag [#2378](https://github.com/starkware-libs/cairo-vm/pull/2378)vm/src/test_helpers/error_utils.rs line 44 at r2 (raw file):
/// Type alias for check functions that validate test results. pub type VmCheck<T> = fn(&std::result::Result<T, CairoRunError>);
can just be Result, right? pls looks for similar instances where you can shorten.
Code quote:
std::result::Resultvm/src/test_helpers/error_utils.rs line 62 at r2 (raw file):
} /// Asserts that the result is `HintError::AssertNotEqualFail`.
This funcs have very repetitive boiler plate. Pls extract to a single func which these func will invoke that will get the res and the predicate u check.
Code quote:
/// Asserts that the result is `HintError::AssertNotEqualFail`.vm/src/test_helpers/error_utils.rs line 220 at r2 (raw file):
} /// `expect_hint_assert_not_zero` does not panic on `HintError::AssertNotZero`.
these error tests could be parameterized using rtest to reduce alot of repetitive boilerplate.
Code quote:
/// `expect_hint_assert_not_zero` does not panic on `HintError::AssertNotZero`.vm/src/test_helpers/test_utils.rs line 48 at r2 (raw file):
Ok(v) => v, Err(e) => panic!("conversion to MaybeRelocatable failed: {e:?}"), };
pls factor out the conversion logic of both cases into a single func which also enforces coercion into MaybeRelocatable (currently it will work for any right that is able to be try_into'd left's type).
Code quote:
let right_mr = match ($right).try_into() {
Ok(v) => v,
Err(e) => panic!("conversion to MaybeRelocatable failed: {e:?}"),
};
TITLE
Description
Description of the pull request changes and motivation.
Checklist
This change is